Skip to content

src: null env_ field from constructor#2913

Closed
trevnorris wants to merge 1 commit intonodejs:masterfrom
trevnorris:fix-my-f-up
Closed

src: null env_ field from constructor#2913
trevnorris wants to merge 1 commit intonodejs:masterfrom
trevnorris:fix-my-f-up

Conversation

@trevnorris
Copy link
Copy Markdown
Contributor

@trevnorris trevnorris commented Sep 16, 2015

The env_ field in ArrayBufferAllocator needs to be null'd out since it
is used during initialization and checked prior to properly being set by
set_env().

Fixes: 74178a5 "buffer: construct Uint8Array in JS"

R=@indutny
R=@Fishrock123

@trevnorris
src: null env_ field from constructor
c573c14 
The env_ field in ArrayBufferAllocator needs to be null'd out since it
is used during initialization and checked prior to properly being set by
set_env().

Fixes: 74178a5 "buffer: construct Uint8Array in JS"
@mscdex mscdex added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 16, 2015
@trevnorris
Copy link
Copy Markdown
Contributor Author

trevnorris commented Sep 16, 2015

CI: https://ci.nodejs.org/job/node-test-pull-request/318/

@indutny
Copy link
Copy Markdown
Member

indutny commented Sep 16, 2015

LGTM, but please wait for CI ;) (see @nodejs/punished )

@rvagg
Copy link
Copy Markdown
Member

rvagg commented Sep 16, 2015

shaming via github team membership 💥

trevnorris added a commit that referenced this pull request Sep 16, 2015
@trevnorris
src: null env_ field from constructor
3b78b85 
The env_ field in ArrayBufferAllocator needs to be null'd out since it
is used during initialization and checked prior to properly being set by
set_env().

Fixes: 74178a5 "buffer: construct Uint8Array in JS"
PR-URL: #2913
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@trevnorris
Copy link
Copy Markdown
Contributor Author

trevnorris commented Sep 16, 2015

Thanks. CI looks good. Landed in 3b78b85.

There are failures of AssertionError: 'Invalid array buffer length' == 'Buffer allocation failed - process out of memory'. These are because the error message thrown from the Uint8Array() constructor is different than the message previously thrown if it failed to allocate memory from C++. Unrelated to this PR.

@trevnorris trevnorris closed this Sep 16, 2015
trevnorris added a commit that referenced this pull request Sep 16, 2015
@trevnorris @Fishrock123
src: null env_ field from constructor
8e58434 
The env_ field in ArrayBufferAllocator needs to be null'd out since it
is used during initialization and checked prior to properly being set by
set_env().

Fixes: 74178a5 "buffer: construct Uint8Array in JS"
PR-URL: #2913
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@trevnorris trevnorris deleted the fix-my-f-up branch September 16, 2015 20:39
@rvagg rvagg mentioned this pull request Sep 22, 2015
Merged
@MylesBorins
Copy link
Copy Markdown
Contributor

MylesBorins commented Nov 16, 2015

landed in lts-v4.x-staging as 8e58434

Also pretty jazzed to find out the history of @nodejs/punished

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@trevnorris @indutny @rvagg @MylesBorins @mscdex @jasnell